Upgrade FXAA #5200
Upgrade FXAA #5200
Conversation
| @@ -50,6 +60,11 @@ define([ | |||
| owner : this | |||
| }); | |||
| this._clearCommand = clearCommand; | |||
|
|
|||
| this._qualityPreset = 39; | |||
There was a problem hiding this comment.
Should these be packed into a vec4 or just constants in the shader?
There was a problem hiding this comment.
I made them shader comments. I wasn't sure if we would expose them. I'm leaving _qualityPreset which is a preprocessor constant because we may expose it or change it for mobile. The possible values are 10-15, 20-29 or 39 (Its documented in a shader comment).
There was a problem hiding this comment.
Sounds good, but did you forget a commit to remove the three properties below?
|
@abwood do you want to look at this? |
|
@emackey do you want to do some before/after quality comparisons? |
|
@bagnell do you have any meaningful before/after performance numbers? How much faster is this? Could be worth mentioning in CHANGES.md that visual quality and performance are slightly better. |
Source/Scene/FXAA.js
Outdated
| this._command = context.createViewportQuadCommand(FXAAFS, { | ||
| var fs = | ||
| '#define FXAA_PC 1\n' + | ||
| '#define FXAA_WEBGL_1 1\n' + |
There was a problem hiding this comment.
Are there WebGL 2 optimizations that we should roadmap in #797? If so, please update that issue.
| @@ -0,0 +1,2074 @@ | |||
| /** | |||
There was a problem hiding this comment.
This file is really long. Our build process will remove comments from release builds, but it will not remove all the versions of this shader that are pulled out at runtime by the preprocessor. I hate to bloat Cesium with that extra code and potentially slow down shader compile times.
Can we either
- Add the preprocessor directives to this file and verify/change our build process to generate the smaller shader, or
- Generate the smaller shader by hand.
In either case, please document how to upgrade this. A comment at the top of the file should be OK.
There was a problem hiding this comment.
I stripped all of the unused shader code. There is a comment at the top that lists the steps. I added the original shader to Source/ThirdParty. It isn't included anywhere so it won't be part of the build, but I can remove it if necessary. There's also a link to the original in the trimmed version after the license.
There was a problem hiding this comment.
Just make this the file in the ThirdParty directory. Git history will have the original and this way we don't have any third-party code outside of the ThirdParty directory.
There was a problem hiding this comment.
If I move it out of the Shaders directory, it won't be part of the build. So it needs to stay there or I can modify the build to check the ThirdParty directory.
|
Does anyone else want to review this or test the visual quality before/after? You might not notice an improvement, we just don't want it to be worse in any case. |
|
I actually did some informal testing yesterday and can confirm that it's no worse. It's not really much better either though, labels are a little crisper but so are orbit lines so it's kind of a trade off. I don't think the average person would notice a difference at all either way. What I really want to see is New/Old/Off comparisons. Right now I lean towards shutting it off in my use cases because I think having good imagery/text/textures outweighs any jaggies but I'd like to confirm that. No need to hold up this PR though. |
|
For performance, I'm not seeing any difference in frame rate. I'm only seeing an increase of performance in the hundredths or milliseconds. |
|
@pjcozzi This should be ready. |
Source/Scene/FXAA.js
Outdated
| u_fxaaQualityRcpFrame : function() { | ||
| return rcpFrame; | ||
| }, | ||
| u_fxaaQualitySubpix : function() { |
There was a problem hiding this comment.
These three aren't used, right?
There was a problem hiding this comment.
No, I forgot those as well. Removed.
|
Looks good. I also tested release. |
|
Please merge this/master into |
|
Hi, |
|
Thanks for checking this out, @sdegenaar! |
Upgrades FXAA to version 3.11. Improves performance and visual quality for geometry. Slightly improves billboards, labels and imagery, but we'll still need a separate pass for better quality. Also, fixes #1921.